Improve source files downloading with local package detection#18
Improve source files downloading with local package detection#18samastek wants to merge 3 commits intosw360:mainfrom
Conversation
|
I've seen you closed this MR, but I quickly reviewed it nevertheless to provide some hints. ;-) |
| if os.path.exists(args.output_file): | ||
| os.remove(args.output_file) |
There was a problem hiding this comment.
First of all, this might be a topic for a separate PR or issue. But in general, what's the goal of this change? I don't think silently overwriting an existing file is a good idea. Perhaps we want a better error message here to avoid a Traceback output?
| or (MapBom.is_good_match(get_cdx(item, "MapResult")) | ||
| and get_cdx(item, "Sw360SourceFileCheck") != "passed")): | ||
| map_result = get_cdx(item, "MapResult") | ||
| is_missing_from_sw360 = map_result == MapResult.NO_MATCH |
There was a problem hiding this comment.
I get your idea to make this code more readable, but NO_MATCH doesn't necessarily mean it's missing, but only we didn't find it. So better use something like is_not_found or is_no_match 😉
| pattern = f"{package_name}_{version}*" | ||
| matches = glob.glob(os.path.join(pkg_dir, pattern)) | ||
| if matches: | ||
| set_cdx(item, "SourceFileComment", "sources locally available") |
There was a problem hiding this comment.
The SourceFileComment is a description used in SW360 for the uploaded attachment. main() abuses it to understand if the source file exists, but it's not a flag.
|
In general, we should have some reasoning in the commit messages explaining the idea(s) of the change(s). As you already found out, the code is not really self explaining, neither are changes to it. ;-) But we probably better start with an issue to discuss a possible change and then you already have the necessary reasoning for the commit message when we've agreed. ;-) |
2 participants
The source download process now checks for locally available source packages.
If these are present, it adds a sourceFileComment indicating that the package exists locally.